Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Store NtLiteral without generalizing to Expr #92392

Closed
wants to merge 1 commit into from
Closed

Store NtLiteral without generalizing to Expr #92392

wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 29, 2021 •
edited
Loading

The discussion under #91166 raised the question of whether a NtLiteral (the value of a $:literal matcher in macro_rules) can be conceptualized as a single token, the way that $:ident is. Today it isn't — a negative literal is passed to procedural macros as a pair of proc macro tokens:

// main.rs

macro_rules! fwd {
    ($literal:literal) => {
        repro!($literal);
    };
}

fwd!(-1);
// lib.rs

#[proc_macro]
pub fn repro(input: TokenStream) -> TokenStream {
    println!("{:#?}", input);
    TokenStream::new()
}
TokenStream [
    Group {
        delimiter: None,
        stream: TokenStream [
            Punct {
                ch: '-',
                spacing: Alone,
            },
            Literal {
                kind: Integer,
                symbol: "1",
                suffix: None,
            },
        ],
    },
]

However proc macro literals already support negative literals in a single token, and it's easy to construct one: proc_macro::Literal::i32_unsuffixed(-1). As such, given that macro_rules's $:literal and proc_macro::Literal are both already capable of representing negative literals in one piece, I think having those literals fall apart on entry into the proc macro is not a great behavior. My expectation would be that the proc macro call in the snippet above would pass a single token which is equivalent to proc_macro::Literal::i32_unsuffixed(-1), and I'd like to start making changes in that direction.

This PR does not make any intentional observable behavior change, but it replaces the way that $:literal is represented internally from P<Expr> (which happens to be always either ExprKind::Lit or ExprKind::Unary(UnOp::Neg, ExprKind::Lit) if rustc is correctly implemented) to a dedicated SignedLiteral type. This makes illegal states unrepresentable and will make it more straightforward to map $:literal one-to-one to proc_macro::Literal whenever that comes up.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 29, 2021
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 29, 2021
@petrochenkov
Copy link
Contributor

r? @petrochenkov

petrochenkov
petrochenkov reviewed Jan 1, 2022
View reviewed changes
Copy link
Contributor

@petrochenkov petrochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to check some things before proceeding with changes in this area, cc #92472.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2022
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 9, 2022
@dtolnay dtolnay mentioned this pull request Jan 19, 2022
Closed
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 22, 2022 •
edited
Loading

Some random thoughts:

  • I think it's just fundamentally wrong to represent fragments like 
    - /* any whitespace tokens including comments possibly with
      newlines */ 1
    as a single token, and the literal matcher accepts such fragments.
  • Literal::i32_unsuffixed and friends were a mistake, IMO. During the major pre-stabilization proc macro API audit (Review proc macro API 1.2 #50473) I remember having something against them, but I don't remember why exactly I didn't actively tried to block their stabilization in their current - accepting form, maybe I thought it was too breaking. The rustc parser used separate tokens for - and 1 at that point, and those methods deliberately introduced a mismatch between internal tokens (also used for macro_rules) and proc macro tokens in the direction of less fine grained proc macro tokens, while the rest of the API made tokens more fine grained compared to the internal version, if there was a mismatch at all.
  • I don't want the internal tokens to have both unglued literal forms (coming from the lexer), and glued forms (coming from Literal::i32_unsuffixed and friends), normalizing at proc macro boundary seems preferable.
  • The plan in (Accept tuple.0.0 as tuple indexing (take 2) #71322) assumed that the internal literal tokens will become more fine-grained, with floating point literals being broken into multiple tokens. That again would make the idea of literal matcher always being a single token incorrect even if we are gluing minuses into the literal tokens now.
  • I don't really like this PR as a refactoring, yes it makes impossible state unrepresentable, but it makes the logic of working with literal fragments more complex and ugly either.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2022
@dtolnay
Copy link
Member Author

dtolnay commented Jan 25, 2022

Thanks for the review! I can tell I'm not going to change your mind on this so I'll go ahead and close.

@dtolnay dtolnay closed this Jan 25, 2022
@dtolnay dtolnay deleted the signedlit branch January 25, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dtolnay @rust-highfive @petrochenkov @davidtwco @rustbot